-
-
Notifications
You must be signed in to change notification settings - Fork 724
chore(oxlint/napi): add createWorkspace & destroyWorkspace js callback function
#16121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore(oxlint/napi): add createWorkspace & destroyWorkspace js callback function
#16121
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #16121 will not alter performanceComparing Summary
Footnotes
|
cc187d9 to
4a61876
Compare
4a61876 to
d05b90b
Compare
bf3b963 to
9c10d86
Compare
1f793f4 to
fda5042
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds workspace management capabilities to the oxlint NAPI interface by introducing createWorkspace and destroyWorkspace JavaScript callback functions. The changes enable isolated workspaces for linting plugins, where each workspace maintains its own set of loaded plugins and rules based on a workspace root directory. This involves threading the workspace directory (cwd) as a parameter throughout the Rust and TypeScript codebases.
Key changes:
- Added
createWorkspaceanddestroyWorkspacecallback types and implementations to manage workspace lifecycle - Modified
Linterstruct to include acwdfield and updated all instantiation sites - Extended
ConfigStoreBuilder::from_oxlintrcto accept acwdparameter for workspace-aware plugin loading - Converted
registeredPluginUrlsandregisteredRulesfrom simple data structures to Maps keyed by workspace directory
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
tasks/benchmark/benches/linter.rs |
Added empty string as cwd parameter to Linter::new call |
napi/playground/src/lib.rs |
Added dummy /root path parameters for ConfigStoreBuilder::from_oxlintrc and Linter::new |
crates/oxc_linter/src/tester.rs |
Added /root path parameters to test linter initialization |
crates/oxc_linter/src/lib.rs |
Added cwd field to Linter struct and updated constructor signature |
crates/oxc_linter/src/external_linter.rs |
Added CreateWorkspaceCb and DestroyWorkspaceCb types, updated ExternalLinter struct |
crates/oxc_linter/src/config/config_builder.rs |
Added cwd parameter to from_oxlintrc and plugin loading functions |
crates/oxc_language_server/src/linter/server_linter.rs |
Updated linter initialization to pass root path as cwd |
crates/oxc_language_server/src/linter/isolated_lint_handler.rs |
Added cwd parameter to handler initialization |
apps/oxlint/src/run.rs |
Added JsCreateWorkspaceCb and JsDestroyWorkspaceCb type definitions, updated lint function signature |
apps/oxlint/src/lint.rs |
Integrated workspace creation in CliRunner::new, threaded cwd through configuration |
apps/oxlint/src/js_plugins/external_linter.rs |
Implemented wrapper functions for workspace creation/destruction callbacks |
apps/oxlint/src-js/plugins/workspace.ts |
New file implementing workspace management with create/destroy functions |
apps/oxlint/src-js/plugins/load.ts |
Modified plugin loading to be workspace-aware using Maps instead of Sets |
apps/oxlint/src-js/plugins/lint.ts |
Updated linting to retrieve rules per workspace directory |
apps/oxlint/src-js/plugins/index.ts |
Exported workspace management functions |
apps/oxlint/src-js/cli.ts |
Added wrapper functions and integrated workspace callbacks into CLI |
apps/oxlint/src-js/bindings.d.ts |
Added TypeScript type definitions for workspace callbacks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fda5042 to
ce53742
Compare
ce53742 to
8a4ded6
Compare
| if let Some(extern_linter) = external_linter { | ||
| let _ = (extern_linter.create_workspace)(self.cwd.to_string_lossy().to_string()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workspace creation failure is silently ignored. If create_workspace fails, subsequent operations that depend on the workspace (like loadPlugin and lintFile) will fail with cryptic errors.
if let Some(extern_linter) = external_linter {
(extern_linter.create_workspace)(self.cwd.to_string_lossy().to_string())?;
}The ? operator should be used to propagate the error, or the error should be logged/handled appropriately.
| if let Some(extern_linter) = external_linter { | |
| let _ = (extern_linter.create_workspace)(self.cwd.to_string_lossy().to_string()); | |
| } | |
| if let Some(extern_linter) = external_linter { | |
| (extern_linter.create_workspace)(self.cwd.to_string_lossy().to_string())?; | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| } | ||
|
|
||
| registeredRules.push(ruleDetails); | ||
| registeredRules.get(workspaceDir)?.push(ruleDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent failure when pushing rule. If the workspace doesn't exist, the optional chaining causes push to not execute, but no error is thrown. This creates inconsistent state where offset is calculated on line 155 assuming the workspace exists, but the rule is never actually registered.
const rules = registeredRules.get(workspaceDir);
if (!rules) {
throw new Error(`Workspace "${workspaceDir}" has not been created`);
}
rules.push(ruleDetails);The workspace should be validated to exist, or an error should be thrown.
| registeredRules.get(workspaceDir)?.push(ruleDetails); | |
| const rules = registeredRules.get(workspaceDir); | |
| if (!rules) { | |
| throw new Error(`Workspace "${workspaceDir}" has not been created`); | |
| } | |
| rules.push(ruleDetails); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.

Adds
createWorkspaceanddestroyWorkspacefor handling multiple "cwd" at the same time.This concept of workspace is later used for the language server for starting / stopping the
ServerLinterfor a workspace.I would much prefer something like
createWorkspace(rootDir:string): { lintFile(), loadPlugin() }, but I am not sure if this is really possible withnapi. Added instead for every function one extra parameter to define the workspace.At the moment, only plugins and rules are stored for each workspace separately.
You can see in the downstream PR how the concept is used when starting / restarting a workspace :)